sdk: fix ConsoleSpanExporter #455

mauriciovasquezbernal · 2020-03-02T13:10:46Z

19d573a ("Add io and formatter options to console exporter (#412)")
changed the way spans are printed by using write() instead of print().
In Python 3.x sys.stdout is line-buffered, so the spans were not being printed
to the console at the right timing.

This commit fixes that by adding an explicit flush() call at the end of the
export function , it also changes the default formatter to include a line break.

To be precise, only one of the changes was needed to solve the problem, but as
a matter of completness both are included, i.e, to handle the case where the
formatter chosen by the user doesn't append a line break.

Fixes: #448
Supersedes: #451

19d573a ("Add io and formatter options to console exporter (open-telemetry#412)") changed the way spans are printed by using write() instead of print(). In Python 3.x sys.stdout is line-buffered, so the spans were not being printed to the console at the right timing. This commit fixes that by adding an explicit flush() call at the end of the export function , it also changes the default formatter to include a line break. To be precise, only one of the changes was needed to solve the problem, but as a matter of completness both are included, i.e, to handle the case where the formatter chosen by the user doesn't append a line break.

codecov-io · 2020-03-02T13:23:39Z

Codecov Report

Merging #455 into master will decrease coverage by 0.14%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #455      +/-   ##
==========================================
- Coverage    88.8%   88.66%   -0.15%     
==========================================
  Files          42       42              
  Lines        2109     2143      +34     
  Branches      238      246       +8     
==========================================
+ Hits         1873     1900      +27     
- Misses        166      170       +4     
- Partials       70       73       +3

Impacted Files	Coverage Δ
...sdk/src/opentelemetry/sdk/trace/export/__init__.py	`89.36% <100%> (+0.15%)`	⬆️
...dk/src/opentelemetry/sdk/metrics/export/batcher.py	`92.5% <0%> (-4.87%)`	⬇️
...ry-sdk/src/opentelemetry/sdk/resources/__init__.py	`68.18% <0%> (-2.66%)`	⬇️
...etry-sdk/src/opentelemetry/sdk/metrics/__init__.py	`94.33% <0%> (-2.44%)`	⬇️
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py	`67.59% <0%> (-0.59%)`	⬇️
...opentelemetry/sdk/context/propagation/b3_format.py	`86.79% <0%> (-0.49%)`	⬇️
...lemetry-sdk/src/opentelemetry/sdk/util/__init__.py	`85.54% <0%> (-0.35%)`	⬇️
...ts/src/opentelemetry/ext/http_requests/__init__.py	`89.18% <0%> (-0.29%)`	⬇️
...src/opentelemetry/ext/opentracing_shim/__init__.py	`95.76% <0%> (-0.18%)`	⬇️
...xt-jaeger/src/opentelemetry/ext/jaeger/__init__.py	`86.39% <0%> (-0.16%)`	⬇️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 11467c4...9d19288. Read the comment docs.

codeboten

Nice one, thanks for fixing!

c24t

Good catch @mauriciovasquezbernal, thanks!

c24t · 2020-03-02T20:10:41Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py

@@ -270,12 +270,14 @@ class ConsoleSpanExporter(SpanExporter):
    def __init__(
        self,
        out: typing.IO = sys.stdout,
-        formatter: typing.Callable[[Span], str] = str,
+        formatter: typing.Callable[[Span], str] = lambda span: str(span)
+        + "\n",
    ):
        self.out = out
        self.formatter = formatter

    def export(self, spans: typing.Sequence[Span]) -> SpanExportResult:
        for span in spans:
            self.out.write(self.formatter(span))


Consider doing this to avoid two linebreaks for formatters that do end in a newline:

Suggested change

self.out.write(self.formatter(span))

self.out.write(endline(self.formatter(span)))

with this in the module:

def endline(line): if line.endswith(os.linesep): return line return line + os.linesep

I haven't checked for windows, but suspect you do want os.linesep here instead of "\n".

I'm not totally sure about this one, I think we should let full control to the user, for instance, there could be a case where the user doesn't want to have line breaks at all and use another separator.

I totally agree about the os.linesep, I'm too focus on Linux now 😝

Btw, if a formatter ends with a linebreak there would not be two linebreaks, I only adding a linebreak to the default formatter.

I think agreed to not force lineseps, formatter should control that (and the flush handles this anyway).

c24t · 2020-03-02T20:11:37Z

opentelemetry-sdk/src/opentelemetry/sdk/trace/export/__init__.py

+        formatter: typing.Callable[[Span], str] = lambda span: str(span)
+        + "\n",


Suggested change

formatter: typing.Callable[[Span], str] = lambda span: str(span)

+ "\n",

formatter: typing.Callable[[Span], str] = str,

Along with the other comment suggestion only.

toumorokoshi

nice!

Rename TracerSource to TracerProvider (open-telemetry#441) Following discussion in open-telemetry#434, align the name with the specification. Co-authored-by: Chris Kleinknecht <[email protected]> Fix new ext READMEs (open-telemetry#444) Some of the new ext packages had ReStructuredText errors. PyPI rejected the uploads for these packages with: HTTPError: 400 Client Error: The description failed to render for 'text/x-rst'. See https://pypi.org/help/#description-content-type for more information. for url: https://upload.pypi.org/legacy/ Adding attach/detach methods as per spec (open-telemetry#429) This change updates the Context API with the following: - removes the remove_value method - removes the set_current method - adds attach and detach methods Fixes open-telemetry#420 Co-authored-by: Chris Kleinknecht <[email protected]> Make Counter and MinMaxSumCount aggregators thread safe (open-telemetry#439) OT Collector trace exporter (open-telemetry#405) Based on the OpenCensus agent exporter. Fixes open-telemetry#343 Co-authored-by: Chris Kleinknecht <[email protected]> API: Renaming TraceOptions to TraceFlags (open-telemetry#450) Renaming TraceOptions to TraceFlags, which is the term used to describe the flags associated with the trace in the OpenTelemetry specification. Closes open-telemetry#434 api: Implement "named" meters + Remove "Batcher" from Meter constructor (open-telemetry#431) Implements open-telemetry#221. Also fixes open-telemetry#394. Stateful.py and stateless.py in metrics example folder are not changed to use the new loader in anticipation of open-telemetry#422 being merged first and removing them. Lastly, moves InstrumentationInfo from trace.py in the sdk to utils. Prepare to host on readthedocs.org (open-telemetry#452) sdk: Implement observer instrument (open-telemetry#425) Observer instruments are used to capture a current set of values at a point in time [1]. This commit extends the Meter interface to allow to register an observer instrument by pasing a callback that will be executed at collection time. The logic inside collection is updated to consider these instruments and a new ObserverAggregator is implemented. [1] https://github.com/open-telemetry/opentelemetry-specification/blob/master/specification/api-metrics.md#observer-instruments sdk: fix ConsoleSpanExporter (open-telemetry#455) 19d573a ("Add io and formatter options to console exporter (open-telemetry#412)") changed the way spans are printed by using write() instead of print(). In Python 3.x sys.stdout is line-buffered, so the spans were not being printed to the console at the right timing. This commit fixes that by adding an explicit flush() call at the end of the export function , it also changes the default formatter to include a line break. To be precise, only one of the changes was needed to solve the problem, but as a matter of completness both are included, i.e, to handle the case where the formatter chosen by the user doesn't append a line break. jaeger: Usage README Update for opentelemetry-ext-jaeger (open-telemetry#459) Usage docs for opentelemetry-ext-jaeger need to be updated after the change to `TracerSource` with v0.4. Looks like it was partially updated already. Users following the usage docs will currently run into this error: `AttributeError: 'Tracer' object has no attribute 'add_span_processor'` api: Implementing Propagators API to use Context (open-telemetry#446) Implementing Propagators API to use Context. Moving tracecontexthttptextformat to trace/propagation, as TraceContext is specific to trace rather that broader context propagation. Using attach/detach for wsgi and flask extensions, enabling activation of the full context rather that activating of a sub component such as traces. Adding a composite propagator. Co-authored-by: Mauricio Vásquez <[email protected]>

mauriciovasquezbernal requested a review from a team March 2, 2020 13:10

knabben approved these changes Mar 2, 2020

View reviewed changes

mauriciovasquezbernal mentioned this pull request Mar 2, 2020

ConsoleSpanExporter doesn't print out as expected #448

Closed

codeboten approved these changes Mar 2, 2020

View reviewed changes

c24t approved these changes Mar 2, 2020

View reviewed changes

mauriciovasquezbernal added 3 commits March 2, 2020 21:07

use os.linesep instead of \n

17bfd52

fix import order

4b214d8

use os.linesep also in tests

9d19288

toumorokoshi approved these changes Mar 4, 2020

View reviewed changes

toumorokoshi merged commit 005575e into open-telemetry:master Mar 4, 2020

mauriciovasquezbernal mentioned this pull request Mar 6, 2020

Use simple processor in http examples #451

Closed

mauriciovasquezbernal mentioned this pull request Mar 12, 2020

ConsoleSpanExporter export is ambiguous/incomplete #478

Closed

mauriciovasquezbernal deleted the mauricio/fix-console-span-exporter branch April 14, 2020 21:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sdk: fix ConsoleSpanExporter #455

sdk: fix ConsoleSpanExporter #455

mauriciovasquezbernal commented Mar 2, 2020

codecov-io commented Mar 2, 2020 •

edited

Loading

codeboten left a comment

c24t left a comment

c24t Mar 2, 2020

mauriciovasquezbernal Mar 3, 2020

mauriciovasquezbernal Mar 3, 2020

toumorokoshi Mar 4, 2020

c24t Mar 2, 2020

toumorokoshi left a comment

	self.out.write(self.formatter(span))
	self.out.write(endline(self.formatter(span)))

		formatter: typing.Callable[[Span], str] = lambda span: str(span)
		+ "\n",

sdk: fix ConsoleSpanExporter #455

sdk: fix ConsoleSpanExporter #455

Conversation

mauriciovasquezbernal commented Mar 2, 2020

codecov-io commented Mar 2, 2020 • edited Loading

Codecov Report

codeboten left a comment

Choose a reason for hiding this comment

c24t left a comment

Choose a reason for hiding this comment

c24t Mar 2, 2020

Choose a reason for hiding this comment

mauriciovasquezbernal Mar 3, 2020

Choose a reason for hiding this comment

mauriciovasquezbernal Mar 3, 2020

Choose a reason for hiding this comment

toumorokoshi Mar 4, 2020

Choose a reason for hiding this comment

c24t Mar 2, 2020

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

codecov-io commented Mar 2, 2020 •

edited

Loading